Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Grid] ignore RSC error #233

Merged
merged 2 commits into from
Sep 17, 2024
Merged

[Grid] ignore RSC error #233

merged 2 commits into from
Sep 17, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Sep 12, 2024

closes mui/material-ui#43635

This is a temporary fix, which might not cover an edge case if the consumer use client component as a child of Grid container.

However, this fix should work for normal use cases because a Grid container should always have Grid item.

@siriwatknp siriwatknp added bug 🐛 Something doesn't work component: Grid The React component. labels Sep 12, 2024
@DiegoAndai
Copy link
Member

Thanks for taking this @siriwatknp.

The solution doesn't work for this case: https://mui.com/material-ui/react-grid2/#inheriting-columns. For that use case to work, a grid item must forward its parent column and spacing values, which won't work if we check for the container prop.

What do you think about something like this?:

function isGridComponent(element) {
  try {
    // For server components `muiName` is avaialble in element.type._payload.value.muiName
    // relevant info - https://github.com/facebook/react/blob/2807d781a08db8e9873687fccc25c0f12b4fb3d4/packages/react/src/ReactLazy.js#L45
    // eslint-disable-next-line no-underscore-dangle
    return element.type.muiName === 'Grid' || element.type?._payload?.value?.muiName === 'Grid';
  } catch {
    // Covers for the case in which the parent is a server component and the child is a client component
    // https://github.com/mui/material-ui/issues/43635
    return false
  };
}

Would this work? It would fail silently, but maybe we can add a callout on the docs about this case.

We should also catch only this specific error and not others.

@siriwatknp

This comment was marked as outdated.

@siriwatknp
Copy link
Member Author

What do you think about something like this?:

You proposal works! I will go with your proposal.

// relevant info - https://github.com/facebook/react/blob/2807d781a08db8e9873687fccc25c0f12b4fb3d4/packages/react/src/ReactLazy.js#L45
// eslint-disable-next-line no-underscore-dangle
return element.type.muiName === 'Grid' || element.type?._payload?.value?.muiName === 'Grid';
} catch (error) {
Copy link
Member

@DiegoAndai DiegoAndai Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check for the specific error so we don't catch unintended ones?

if (error.message.startsWith("Cannot access default.muiName on the server.") {
  // Covers for the case in which the Grid is a server component and the child is a client component
  // https://github.com/mui/material-ui/issues/43635
  return false;
} else {
  throw error
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure, the message check is quite fragile. Given the size of the function, eventhough it catches, I think it's still better than checking message.

cc @brijeshb42

Copy link
Contributor

@brijeshb42 brijeshb42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just speculating. Is there some way we can also give this control to the user so they can customize this if the error criteria changes in the future?

@siriwatknp siriwatknp changed the title [Grid] only check child for Grid container [Grid] only check child for Grid Sep 17, 2024
@siriwatknp siriwatknp changed the title [Grid] only check child for Grid [Grid] ignore RSC error Sep 17, 2024
@siriwatknp siriwatknp merged commit e45688d into master Sep 17, 2024
18 checks passed
@siriwatknp siriwatknp deleted the fix/grid-isGridComponent branch September 17, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Grid The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-pigment-css] Grid throws an error when it is RSC
3 participants